Skip to content

feat: Add wrapper option to render #173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
May 28, 2019
Merged

feat: Add wrapper option to render #173

merged 6 commits into from
May 28, 2019

Conversation

OzzieOrca
Copy link
Contributor

Summary

Adds wrapper option to render function to allow users to specify a WrapperComponent that will be used by render, update, and rerender.

Resolves #172

Test plan

Added tests. Tested rerender and not update since it is an alias. Could test both or just update instead...

Notes

  1. I was basing this off of native-testing-library https://github.com/testing-library/native-testing-library/blob/master/src/index.js#L16-L23. They are wrapping all tests in AppContainer which I'm not sure why. Maybe to pick up yellow box errors?
  2. I thought their implementation was also hiding the WrappedComponent from errors and debug/snapshot outputs. Pretty sure they aren't doing any such thing and it's just that the Providers I was rendering don't actually have any user facing output. (Still getting used to non-enzyme shallow tests...)
  3. I've never written flow types before and was mostly just copying code over, feel free to suggest/change things to improve this or make it fit the style of the repo.
  4. Feel free to suggest a better way to pass the rest of the options down to TestRenderer.create. I initially tried spreading the rest out but couldn't get flow to cooperate.
  5. Might be useful to link to or provide docs like https://www.native-testing-library.com/docs/setup#custom-render which are a suggestion of how to use this.

@thymikee thymikee requested a review from Esemesek May 17, 2019 07:36
@thymikee
Copy link
Member

Thanks for sending the PR :) We're gonna have a look soon, hopefully tomorrow

@thymikee
Copy link
Member

cc @Esemesek, mind taking a look?

thymikee
thymikee requested changes May 20, 2019
View reviewed changes
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments on the implementation to address. Please update the TypeScript definitions as well :)

src/render.js Outdated
update: updateWithAct(renderer),
rerender: updateWithAct(renderer), // alias for `update`
update: updateWithAct(renderer, wrapUiIfNeeded),
rerender: updateWithAct(renderer, wrapUiIfNeeded), // alias for `update`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update and rerender are wrapped with updateWithAct twice now. One is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't change that but just noticed that and you're right. I'll remove the ones up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading it wrong. You want me to assign it to a const so it's only called once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this is what you had in mind...

@OzzieOrca
Copy link
Contributor Author

Thanks :) I'll take a look. Are the TypeScript definitions written manually?

@thymikee
Copy link
Member

Are the TypeScript definitions written manually

Yep, we write them by hand.

@OzzieOrca
Copy link
Contributor Author

I'd appreciate another review on this. I'm not sure where to go with a couple of the comments. I thought they were just going to be quick fixes but as I dug into it again, I'm not sure the code can be simplified exactly as you proposed. I suggested a couple ideas. Thanks 😃

@thymikee
Copy link
Member

@Esemesek can you help out here please? :)

@thymikee
Copy link
Member

I'm out for React Europe till the end of the week so it may take me some time to review properly again.

@thymikee thymikee changed the title Add wrapper option to render feat: Add wrapper option to render May 28, 2019
@thymikee thymikee merged commit f6f0a70 into callstack:master May 28, 2019
@OzzieOrca
Copy link
Contributor Author

Awesome, thanks guys!

So the obligatory follow up question, could we get this released sometime soon? 😃 Thanks for your work reviewing and cleaning it up and being open to PRs :)

I had let Prettier attack those files so sorry for the reformatting needed.

@OzzieOrca OzzieOrca deleted the render-wrapper branch May 28, 2019 20:40
@thymikee
Copy link
Member

Thank you! It's gonna be released tomorrow morning as v1.8 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thymikee thymikee thymikee approved these changes

@Esemesek Esemesek Esemesek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for wrapper on render
3 participants
@OzzieOrca @thymikee @Esemesek